Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(web-api): add request interceptor and HTTP adapter config to WebClient [ISSUE-2073] #2076

Merged
merged 10 commits into from
Oct 16, 2024
Merged

feat(web-api): add request interceptor and HTTP adapter config to WebClient [ISSUE-2073] #2076

merged 10 commits into from
Oct 16, 2024

Conversation

mtjandra
Copy link
Contributor

Summary

Resolves #2073

Features

  • ability to configure one axios request interceptor
    • useful for clients who would like to manipulate outgoing requests to conform to their custom proxies
  • ability to configure an axios adapter
    • useful for client who would like to used their existing http client
      • ex: client may have a dedicated http client configured to be used with their internal proxy

Contribution Pre-Requisites

@salesforce-cla Salesforce CLA
Copy link

Thanks for the contribution! Before we can merge this, we need @mtjandra to sign the Salesforce Inc. Contributor License Agreement.

@mtjandra mtjandra mentioned this pull request Oct 11, 2024
Closed
@filmaj filmaj self-requested a review October 15, 2024 15:36
@filmaj filmaj closed this Oct 15, 2024
@filmaj filmaj reopened this Oct 15, 2024
@codecov Codecov
Copy link

codecov bot commented Oct 15, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.23656% with 7 lines in your changes missing coverage. Please review.

Project coverage is 91.83%. Comparing base (2af216c) to head (5dabe0f).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2076      +/-   ##
==========================================
+ Coverage   91.75%   91.83%   +0.07%     
==========================================
  Files          38       38              
  Lines       10093    10264     +171     
  Branches      631      646      +15     
==========================================
+ Hits         9261     9426     +165     
- Misses        820      826       +6     
  Partials       12       12
Flag Coverage Δ
cli-hooks 95.23% <ø> (ø)
cli-test 95.69% <ø> (ø)
oauth 77.39% <ø> (ø)
socket-mode 58.22% <ø> (ø)
web-api 96.88% <96.23%> (-0.02%) ⬇️
webhook 96.65% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

filmaj
filmaj requested changes Oct 15, 2024
View reviewed changes
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for the PR! I think this is heading in a great direction. I left many questions for my own education and to help inform my future reviews as well.

One higher-level question: what is the intention behind exposing both an adapter option as well as requestInterceptor? How/why would consumers use either/or? Would they use both in any situation? The request interceptor makes sense to expose, but based on the axios docs, it seems adapter is mainly for use for testing (https://axios-http.com/docs/req_config)?

Also, I think we may need to add something to our docs about this new feature. Looking at the right menu bar for the web-api docs here, perhaps a new section on request interceptors / modifiers would be in order? What do you think?

packages/web-api/src/WebClient.ts Show resolved Hide resolved
packages/web-api/src/WebClient.ts Show resolved Hide resolved
packages/web-api/src/WebClient.ts Show resolved Hide resolved
packages/web-api/src/WebClient.ts Outdated Show resolved Hide resolved
packages/web-api/src/WebClient.spec.ts Outdated Show resolved Hide resolved
packages/web-api/src/WebClient.spec.ts Outdated Show resolved Hide resolved
@filmaj filmaj added this to the [email protected] milestone Oct 15, 2024
@filmaj filmaj added semver:minor enhancement M-T: A feature request for new functionality pkg:web-api applies to `@slack/web-api` labels Oct 15, 2024
@mtjandra mtjandra requested a review from filmaj October 16, 2024 14:53
filmaj
filmaj approved these changes Oct 16, 2024
View reviewed changes
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! Left a few minor documentation suggestions; once applied or rejected, I will merge this in.

Thank you so much for your work here and patience with me during review, it is greatly appreciated 🙇

docs/content/packages/web-api.md Outdated Show resolved Hide resolved
docs/content/packages/web-api.md Outdated Show resolved Hide resolved
docs/content/packages/web-api.md Outdated Show resolved Hide resolved
docs/content/packages/web-api.md Outdated Show resolved Hide resolved
packages/web-api/src/WebClient.spec.ts Outdated Show resolved Hide resolved
packages/web-api/src/WebClient.ts Outdated Show resolved Hide resolved
packages/web-api/src/WebClient.ts Show resolved Hide resolved
packages/web-api/src/WebClient.ts Outdated Show resolved Hide resolved
@mtjandra mtjandra force-pushed the issue-2073/axios-interceptors branch 5 times, most recently from 1c5eec0 to 2652028 Compare October 16, 2024 19:41
@mtjandra mtjandra force-pushed the issue-2073/axios-interceptors branch from 2652028 to 5dabe0f Compare October 16, 2024 19:45
@mtjandra
Copy link
Contributor Author

Awesome work! Left a few minor documentation suggestions; once applied or rejected, I will merge this in.

Thank you so much for your work here and patience with me during review, it is greatly appreciated 🙇

Thank you for helping me with this and being responsive! I've addressed your comments and have rebased with main

@filmaj filmaj merged commit 8ba3a43 into slackapi:main Oct 16, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@filmaj filmaj filmaj approved these changes

Assignees
No one assigned
Labels
cla:signed enhancement M-T: A feature request for new functionality pkg:web-api applies to `@slack/web-api` semver:minor
Projects
None yet
Milestone
[email protected]
Development

Successfully merging this pull request may close these issues.

web-api: expose axios interceptors as WebClient constructor option
2 participants
@mtjandra @filmaj